Added Pre-Commit hook support #541

Jarmos-san · 2023-07-24T17:26:22Z

Fixes #539

This PR adds the pre-commit hooks configurations and documentation changes to reflect its support.

.pre-commit-hook.yaml

f1rstlady · 2023-07-30T07:37:56Z

Nice work! I had this on my todo list for half a year, decided yesterday to address it and found that you just implemented this last week. What a coincindence!

f1rstlady · 2023-07-30T07:41:46Z

.pre-commit-hooks.yaml

Trying to run this hook via pre-commit try-repo https://github.com/Jarmos-san/selene selene fails with

=============================================================================== Using config: =============================================================================== repos: - repo: https://github.com/Jarmos-san/selene rev: bd8b81cd7ee57727e4bd128a80bbdb1acc83a17f hooks: - id: selene =============================================================================== [WARNING] Unstaged files detected. [INFO] Stashing unstaged files to /tmp/tmpcir77ixa/patch1690702419-3175. [INFO] Initializing environment for https://github.com/Jarmos-san/selene. [INFO] Installing environment for https://github.com/Jarmos-san/selene. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... [INFO] Restored changes from /tmp/tmpcir77ixa/patch1690702419-3175. An unexpected error has occurred: CalledProcessError: command: ('/tmp/tmpcir77ixa/repovkkeuvf0/rustenv-default/bin/cargo', 'install', '--bins', '--root', '/tmp/tmpcir77ixa/repovkkeuvf0/rustenv-default', '--path', '.') return code: 101 stdout: (none) stderr: info: syncing channel updates for 'stable-x86_64-unknown-linux-gnu' info: latest update on 2023-07-13, rust version 1.71.0 (8ede3aae2 2023-07-12) info: downloading component 'cargo' info: downloading component 'clippy' info: downloading component 'rust-docs' info: downloading component 'rust-std' info: downloading component 'rustc' info: downloading component 'rustfmt' info: installing component 'cargo' info: installing component 'clippy' info: installing component 'rust-docs' info: installing component 'rust-std' info: installing component 'rustc' info: installing component 'rustfmt' error: found a virtual manifest at `/tmp/tmpcir77ixa/repovkkeuvf0/Cargo.toml` instead of a package manifest Check the log at /home/bmr/.cache/pre-commit/pre-commit.log

I suspect Cargo expects something different?

Hey thanks for reporting the issue! I did some research and stumbled across rust-lang/cargo#7599 which is why Cargo fails to correctly identify which Cargo.toml to use based on the correct workspace! ~~I think it might be possible to circumvent this issue by specifying the exact path to the correct workspace using the args option of the .pre-commit-hooks.yaml file (see related docs)~~ Specifying --path selene with the args key does not work as I expect it to. I'm kinda out of ideas now since I'm not very familiar with cargo atm.

Besides, I also noticed the other Docker based hook is breaking too! See the error logs I stumbled across:

=============================================================================== Using config: =============================================================================== repos: - repo: https://github.com/Jarmos-san/selene rev: bd8b81cd7ee57727e4bd128a80bbdb1acc83a17f hooks: - id: selene-docker =============================================================================== [WARNING] Unstaged files detected. [INFO] Stashing unstaged files to /tmp/tmp5m_0u5m4/patch1690706713-30953. [INFO] Initializing environment for https://github.com/Jarmos-san/selene. [INFO] Installing environment for https://github.com/Jarmos-san/selene. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... [INFO] Restored changes from /tmp/tmp5m_0u5m4/patch1690706713-30953. An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/docker', 'build', '--tag', 'pre-commit-75061517ae24f03e43af03f990801e33', '--label', 'PRE_COMMIT', '--pull', '.') return code: 1 stdout: (none) stderr: #0 building with "default" instance using docker driver #1 [internal] load .dockerignore #1 transferring context: 2B done #1 DONE 0.8s #2 [internal] load build definition from Dockerfile #2 transferring dockerfile: 1.24kB 0.1s done #2 DONE 0.8s #3 [internal] load metadata for docker.io/library/rust:1.64-alpine3.14 #3 ERROR: docker.io/library/rust:1.64-alpine3.14: not found #4 [internal] load metadata for docker.io/library/bash:latest #4 CANCELED ------ > [internal] load metadata for docker.io/library/rust:1.64-alpine3.14: ------ Dockerfile:17 -------------------- 15 | cargo install --branch main --git https://github.com/Kampfkarren/selene selene 16 | 17 | >>> FROM rust:1.64-alpine3.14 AS selene-light-musl-builder 18 | RUN apk add g++ && \ 19 | cargo install --no-default-features --branch main --git https://github.com/Kampfkarren/selene selene -------------------- ERROR: failed to solve: rust:1.64-alpine3.14: docker.io/library/rust:1.64-alpine3.14: not found Check the log at /home/space/.cache/pre-commit/pre-commit.log

~~Is it because perhaps the rust:1.64-alpine3.14 Image does not exists?~~ I tried building the Image based on the Dockerfile and the build breaks so I'm sure its because the rust:1.64-alpine3.14 wasn't found.

It might be a better idea to open another PR with a fix for the Docker build no?

I agree, the Docker build should be fixed in a separate PR.

Regarding the Cargo error, I'm also not familiar with Cargo. Maybe @Kampfkarren has an idea? pre-commit's installation procedure is documented here.

Would you like to take the initiative to fix the Dockerfile since you already had the idea work on this PR earlier?

I would, if I'd be familiar with containers...

Oh haha...cheers then! Don't worry I'll send another with a fix next weekend then. By then I hope Kamp will help us out figure out how to fix the cargo error.

It looks like we can make it use rust-alpine, right? I always want latest Rust anyway.

Yes, it works with rust:alpine; checked it recently.

Oh great glad rust:alpine works, so @f1rstlady do you want to try sharing a PR with a fixed Dockerfile? I can mark this PR a draft till you have shared a mergeable PR. What do you say?

Or if you want I can look into the current Dockerfile and see if I can fix the broken build.

Here it is: #549

I haven't tested the corresponding pre-commit hook yet since I used podman instead of docker to build the repaired image.

Kampfkarren · 2023-08-27T07:00:46Z

Apologies for the delay, been very busy.

@f1rstlady @Jarmos-san Yes the Docker build is broken...it was submitted by someone else and has not been properly maintained.

amitds1997 · 2023-10-29T05:40:13Z

Any progress on this? If this needs some work, I can try to get it done

Jarmos-san · 2023-10-29T08:24:30Z

Well I wanted to work on it but couldn't find time but please feel free to take it up. I would be equally pleased to have someone implement the Pre-Commit hook other than me.

amitds1997 · 2023-10-30T05:36:53Z

.pre-commit-hooks.yaml

+- id: selene-docker
+  name: selene (docker)
+  description: An opinionated Lua code linter
+  entry: selene


Suggested change

entry: selene

entry: /selene

This should fix the Docker run.

amitds1997 · 2023-10-30T05:42:07Z

I looked into the rust language-based pre-commit hook. It calls cargo install --bins --path . and since we are using workspaces, it errors with error: found a virtual manifest at <repo-path>/Cargo.toml instead of a package manifest. I found a related issue: pre-commit/pre-commit#2931, but the author seems to have opposed the idea of allowing the --path variable to have an input. So, we have to include a workaround in our build process somehow (not sure though, or if there is a workaround). Or, we can go ahead with just the system and the docker hook, for now. They should work.

Any recommendations: @Jarmos-san @Kampfkarren @f1rstlady?

@Jarmos-san If you could provide with me access, I would like to keep this PR open, just to build a continuity for the discussion. Or, I can open a fresh PR? Let me know what would be suitable for you.

Jarmos-san · 2023-10-30T05:52:03Z

@amitds1997 you can reopen a new PR and I will not have no issues with it as long as I can run selene as a pre-commit hook. 😅 Just reference this PR so that I can close it accordingly and use it as a source of reference when I've to reconfigure my existing pre-commit hooks with selene.

f1rstlady · 2023-10-30T09:09:09Z

@Jarmos-san may you merge the main to get the updated docker image?

Jarmos-san · 2023-10-30T09:23:26Z

@f1rstlady do you want me to update this PR's branch with the latest updates from the main branch? I think @amitds1997 is going to fork and share his own PR so I think it will have the latest commits to the main branch any way. Once he does it, I can close this PR in favour of the other one.

f1rstlady · 2023-10-30T09:34:45Z

Ok, now I get your idea!

amitds1997 · 2023-10-30T16:10:55Z

Created #565 for this.

Jarmos-san added 2 commits July 24, 2023 22:53

Update the docs about the pre-commit hook

1b130ba

Add the pre-commit hook configurations

f8501e2

f1rstlady reviewed Jul 29, 2023

View reviewed changes

.pre-commit-hook.yaml Outdated Show resolved Hide resolved

fix the pre-commit hook config filename

bd8b81c

f1rstlady suggested changes Jul 30, 2023

View reviewed changes

Merge branch 'main' into main

f0af99e

Jarmos-san marked this pull request as draft August 27, 2023 08:22

Jarmos-san mentioned this pull request Aug 27, 2023

Fix broken Docker build #550

Closed

amitds1997 reviewed Oct 30, 2023

View reviewed changes

Jarmos-san closed this Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Pre-Commit hook support #541

Added Pre-Commit hook support #541

Jarmos-san commented Jul 24, 2023

f1rstlady commented Jul 30, 2023

f1rstlady Jul 30, 2023

Jarmos-san Jul 30, 2023 •

edited

Loading

f1rstlady Jul 30, 2023

Jarmos-san Jul 30, 2023

f1rstlady Jul 30, 2023 •

edited

Loading

Jarmos-san Jul 30, 2023

Kampfkarren Aug 27, 2023

f1rstlady Aug 27, 2023

Jarmos-san Aug 27, 2023

f1rstlady Aug 27, 2023 •

edited

Loading

Kampfkarren commented Aug 27, 2023

amitds1997 commented Oct 29, 2023

Jarmos-san commented Oct 29, 2023

amitds1997 Oct 30, 2023

amitds1997 commented Oct 30, 2023

Jarmos-san commented Oct 30, 2023

f1rstlady commented Oct 30, 2023

Jarmos-san commented Oct 30, 2023

f1rstlady commented Oct 30, 2023

amitds1997 commented Oct 30, 2023

Added Pre-Commit hook support #541

Added Pre-Commit hook support #541

Conversation

Jarmos-san commented Jul 24, 2023

f1rstlady commented Jul 30, 2023

Choose a reason for hiding this comment

Jarmos-san Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1rstlady Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1rstlady Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Kampfkarren commented Aug 27, 2023

amitds1997 commented Oct 29, 2023

Jarmos-san commented Oct 29, 2023

Choose a reason for hiding this comment

amitds1997 commented Oct 30, 2023

Jarmos-san commented Oct 30, 2023

f1rstlady commented Oct 30, 2023

Jarmos-san commented Oct 30, 2023

f1rstlady commented Oct 30, 2023

amitds1997 commented Oct 30, 2023

Jarmos-san Jul 30, 2023 •

edited

Loading

f1rstlady Jul 30, 2023 •

edited

Loading

f1rstlady Aug 27, 2023 •

edited

Loading